-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Converted tests to use BaseRegTest #52
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a long needed refactoring! Just a doubt: is is alright/consistent to have the same class name TestExtrusion
for all the different test files?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have mixed feelings about the examples being gone, as I think they are useful to run standalone. In some sense they are there, but now wrapped as tests.
That aside, for regression testing, I am not convinced the json data structure is an appropriate one, since eventually we want to compare grid coordinates, which will blow up the file size. In general, I would like to compare more directly the binary output. This means that we would need to store the cgns reference files, but by doing so we retain all the information we ever need to compare, as well as have smaller reference file sizes compared to the json files.
For instance, I think we could probably utilize the existing cgnsdiff
tool with appropriate flags to check and compare the final cgns file against some reference cgns file.
Alternatively, we could read the cgns file in using cgns_utils
and compare the blocksizes directly of the two.
Let me know what you think.
Right, we have some ways of wrapping examples as tests while keeping examples intact. See for example pytacs tests. We can talk offline, but either way it is important to test any examples we keep, because otherwise they may be broken in the future without us knowing.
What about using some sort of reduction on the grid coordinates, such as summing coordinates in each direction? I think this type of thing is done in ADflow and we could do something similar here, without having to store binary files. |
I'll take a look at the pytacs tests and see if we can keep the examples. As for the references, we could store the reference CGNS files on AFS like we do for inputs on other repos. I'll take a look at |
|
Another issue is that we would have to compile CGNS with |
The docker builds do not have cgns tools enabled, so there's some extra work to be done there. |
@sseraj I would be inclined to use |
Thanks for the Docker update. I am going to close this PR and open one that:
|
Purpose
I converted the tests to use
BaseRegTest
instead of callingsubprocess
on the examples and parsing the output. I converted all the examples scripts into test scripts but kept the BWB script so that it could still be included in the tutorial.The test coverage now also includes the boundary conditions, which is one of the additions mentioned in #35. mdolab/cgnsutilities#43 needs to be merged before this PR.
Type of change
What types of change is it?
Select the appropriate type(s) that describe this PR
Testing
Run the tests as before.
Checklist
Put an
x
in the boxes that apply.flake8
andblack
to make sure the code adheres to PEP-8 and is consistently formatted